fix(theming): allow three states in useTheme#656
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #656 +/- ##
==========================================
+ Coverage 75.84% 77.17% +1.33%
==========================================
Files 149 153 +4
Lines 13682 13771 +89
Branches 1040 1114 +74
==========================================
+ Hits 10377 10628 +251
+ Misses 3299 3139 -160
+ Partials 6 4 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| File | Base | Head | Diff |
|---|---|---|---|
orama-db.json |
8.05 MB | 8.05 MB | +1.63 KB (+0.02%) |
There was a problem hiding this comment.
Pull request overview
Refactors the web generator UI theming to track a theme preference (including system) and keep the applied document theme synced with OS color-scheme changes, updating the NavBar integration accordingly.
Changes:
- Reworked
useThemeto supportsystem | light | darkpreferences, with localStorage persistence and system-change listeners. - Updated
NavBarto pass the new theme preference API through toThemeToggle.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/generators/web/ui/hooks/useTheme.mjs |
Introduces theme preference model (system/light/dark), storage helpers, and OS theme change syncing. |
src/generators/web/ui/components/NavBar.jsx |
Switches NavBar theme toggle wiring to the new useTheme return values/props. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Could you please elaborate more why this PR is necessary? Why does it need to be refactored? Is this fixing any specific bug? 👀 |
In the linked issue we realize that on safari theme didn't change on web generator when using the dropdown so after some thinkering I found this solution |
ovflowd
left a comment
There was a problem hiding this comment.
I might be getting crazy but this hook is much bigger than I genuinely imagine we need for a simple useTheme hook 🤔
what if we use |
It's a bit complex as it is, sure, but it can be simplified. For instance, the following achieves the same result (Note this removes the legacy import { useState, useEffect, useCallback } from 'react';
/** @returns {'dark'|'light'} The current OS-level color scheme. */
const getSystemTheme = () =>
matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light';
/**
* Applies a theme to the document root.
* Resolves 'system' to the actual OS preference before applying.
* @param {'system'|'light'|'dark'} pref - The theme preference.
*/
const applyTheme = (pref) => {
const theme = pref === 'system' ? getSystemTheme() : pref;
document.documentElement.setAttribute('data-theme', theme);
document.documentElement.style.colorScheme = theme;
};
/**
* Applies the system theme to the document root.
*/
const applySystemTheme = () => applyTheme('system');
/**
* React hook for managing theme preference.
* Persists the choice to localStorage and listens for OS theme changes
* when set to 'system'.
* @returns {['system'|'light'|'dark', (next: 'system'|'light'|'dark') => void]}
*/
export const useTheme = () => {
// Read stored preference once on mount; default to 'system'.
const [pref, setPref] = useState(() => {
return localStorage.getItem('theme') || 'system';
});
// Apply theme on every preference change, and if 'system',
// also listen for OS-level color scheme changes.
useEffect(() => {
applyTheme(pref);
if (pref !== 'system') return;
const mql = matchMedia('(prefers-color-scheme: dark)');
mql.addEventListener('change', applySystemTheme);
return () => mql.removeEventListener('change', applySystemTheme);
}, [pref]);
/** Updates the preference in both React state and localStorage. */
const setTheme = useCallback((next) => {
setPref(next);
localStorage.setItem('theme', next);
}, []);
return [pref, setTheme];
}; |
Co-Authored-By: Aviv Keller <me@aviv.sh>
Co-Authored-By: Aviv Keller <me@aviv.sh>
|
Bump @AugustinMauroy |
|
Note: This doesn't resolve #642, which is a legacy generator issue. This resolves a similar issue in the new generator. |
i was waiting answer here #656 (comment) |
|
I’m considering this change a hot-fix, so I dismissed Claudio’s outdated review |
Description
refracto the way that we handle theming
Validation
Manual test on safari & chrome
Check List
node --run testand all tests passed.node --run format&node --run lint.